Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixture: change hbs rules to allow template class name formation without dashes and some typos #2481

Merged
merged 1 commit into from
Sep 3, 2019
Merged

Conversation

SEWeiTung
Copy link
Contributor

@SEWeiTung SEWeiTung commented Aug 30, 2019

Now if you run with npm run test locally, we've got some errors of
page checkings:

  1. layouts/partials/footer.hbs: line 3, col 20, value must match the
    format: dash

=> This problem is caused by the special formation in {{...}}, because
this is an hbs file, just like Angular or some template pages, we
SHOULDN'T check its variable name, so ignore it by setting:
id-class-ignore-regex": "{{.*?}}" in the '.htmllintrc'.

  1. layouts/partials/footer.hbs: line 3, col 1, tag is not closed

=> Add spaces between some tag names.

  1. layouts/partials/navigation.hbs: line 6, col 11, tag is not closed

=> Add spaces between some tag names.

  1. layouts/partials/header.hbs: line 10, col 9, tag is not closed

=> Add spaces between some tag names.

Now after this fixture, it looks nice:
image

@SEWeiTung SEWeiTung requested a review from a team August 30, 2019 06:30
@yous
Copy link
Contributor

yous commented Aug 30, 2019

In my environment, npm run test runs htmllint against only 18 files. Can you also correct the script?

diff --git a/package.json b/package.json
index f1da085d..6d806381 100644
--- a/package.json
+++ b/package.json
@@ -14,7 +14,7 @@
     "load-schedule": "curl -sS https://raw.githubusercontent.com/nodejs/Release/master/schedule.json -o source/schedule.json",
     "start": "npm run serve",
     "test": "npm run test:lint && npm run test:unit && npm run test:smoke",
-    "test:lint": "standard --fix && htmllint **/*.hbs && stylint layouts/css && markdownlint \"**/*.md\" -i \"node_modules/\"",
+    "test:lint": "standard --fix && htmllint \"**/*.hbs\" && stylint layouts/css && markdownlint \"**/*.md\" -i \"node_modules/\"",
     "test:unit": "tape tests/**/*.test.js | faucet",
     "test:smoke": "tape tests/*.smoketest.js | faucet"
   },

layouts/partials/footer.hbs Outdated Show resolved Hide resolved
Now if you run with `npm run test` locally, we've got some errors of
page checkings:

1. layouts/partials/footer.hbs: line 3, col 20, value must match the
format: dash
=> This problem is caused by the special formation in {{...}}, because
this is an hbs file, just like Angular or some template pages, we
SHOULDN'T check its variable name, so ignore it by setting:
id-class-ignore-regex": "{{.*?}}" in the '.htmllintrc'.

2. layouts/partials/footer.hbs: line 3, col 1, tag is not closed
=> Add spaces between some tag names.

3. layouts/partials/navigation.hbs: line 6, col 11, tag is not closed
=> Add spaces between some tag names.

4. layouts/partials/header.hbs: line 10, col 9, tag is not closed
=> Add spaces between some tag names.
@SEWeiTung SEWeiTung requested a review from yous August 30, 2019 07:19
@XhmikosR
Copy link
Contributor

Please do not add the spaces just because a random tool complains.

This was intentionally changed in 71488cd

@XhmikosR
Copy link
Contributor

I don't want to sound too harsh, but htmllint is pretty much useless IMO. It doesn't catch real issues. See also #2459 and #2469 for a proper HTML checking tool.

@Trott
Copy link
Member

Trott commented Aug 30, 2019

Please do not add the spaces just because a random tool complains.

This was intentionally changed in 71488cd

In a way, this comes down to values. With no difference in rendering or user experience, do you value pretty formatting of the final HTML output (in which case you get rid of the space) or do you value readabaility/maintainability of the template files for developers (in which case you add the space)?

In this particular case, I don't care much, but I suppose if forced to make a choice, I would say the template readability is more important than avoiding an extra unsightly-but-harmless space in the HTML tag. So I'd be for adding the space. But I don't care enough to argue the point if others disagree.

@SEWeiTung
Copy link
Contributor Author

Please do not add the spaces just because a random tool complains.

This was intentionally changed in 71488cd

Why? What do you mean by 'random tool complains'? Any other errors happen?

@XhmikosR
Copy link
Contributor

All I'm saying is that we shouldn't do what we are doing.

Even if httmllint was a better tool (which in our case it's proven it's not), we aren't using it properly. These are handlebars templates, not HTML files.

Anyway, you guys do whatever you prefer. For me, what we have on master is fine and readable.

@SEWeiTung
Copy link
Contributor Author

All I'm saying is that we shouldn't do what we are doing.
Even if httmllint was a better tool (which in our case it's proven it's not), we aren't using it properly. These are handlebars templates, not HTML files.
Anyway, you guys do whatever you prefer. For me, what we have on master is fine and readable.

Ah ha, I'm afraid you've misunderstood me, so please don't be worried or angry. What I mean doesn't challenge you but just wanna know your real reason——If you have enough reason to tell us and that looks good, maybe we can dismiss this submit and follow your ideas, this is only the place where we can chat happily ;)

@SEWeiTung SEWeiTung merged commit 2f6087c into nodejs:master Sep 3, 2019
@SEWeiTung SEWeiTung deleted the FixFooterHbsFormation branch September 3, 2019 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants